loadConfig() falls back to ~/.gsd/defaults.json when no project config exists#1699
loadConfig() falls back to ~/.gsd/defaults.json when no project config exists#1699rodzved wants to merge 2 commits intogsd-build:mainfrom
Conversation
|
Update: Refactored the Before: Each config key was listed explicitly in the return object (29 lines), meaning any new key added to After: The fallback iterates over All 2159 tests still pass. |
|
This PR needs a rebase onto CI checks are also missing — likely due to the stale base. |
… config exists Closes #1683
Avoids maintenance trap where new config keys added to the try block would need to be duplicated in the catch fallback path.
rodzved
force-pushed
the
enhancement/loadconfig-defaults-fallback
branch
from
994d675 to
f7878f4
Compare
|
Rebased onto main to pick up #1708 ( |
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Review: loadConfig() defaults.json fallback
Verdict: Request Changes
Merge conflict status
The needs merge fixes label appears stale -- GitHub reports this PR as MERGEABLE. PR #1708 (CONFIG_DEFAULTS refactor, commit 7712595) has since merged and extracted the inline defaults into CONFIG_DEFAULTS. Because the catch-block change is in a separate hunk from the constant extraction, git can auto-merge. However, the author should rebase onto main and confirm the catch block references CONFIG_DEFAULTS (via const defaults = CONFIG_DEFAULTS) rather than the old inline object, then re-run tests to verify.
Issues found
1. Side-effect write in a read function (design concern)
loadConfig() is a read-path function, but the depth-to-granularity migration writes back to ~/.gsd/defaults.json (line ~377 in the diff). This creates a race condition if multiple GSD commands call loadConfig() concurrently (e.g., parallel subagents). The migration write-back should be removed from loadConfig() and handled by a dedicated migration path, or at minimum documented as intentional. The same migration already exists in buildNewProjectConfig() in config.cjs -- loadConfig() should read-only and let the existing migration handle persistence.
Suggested fix: Remove the fs.writeFileSync call in the catch block. Keep the in-memory depth -> granularity remap so the returned config is correct, but do not persist the change from within loadConfig().
2. Missing test for depth-to-granularity migration in the fallback path
The depth migration logic in the catch block (lines ~373-379) has no test coverage. Add a test that places { "depth": "comprehensive" } in defaults.json and asserts config.granularity === 'fine'.
3. CHANGELOG.md not updated
The PR checklist shows CHANGELOG.md is unchecked. This is a user-facing behavior change (pre-project commands now read global defaults) and should have an entry.
What looks good
- Correct use of
node:test/node:assert/strict/ CommonJS -- style compliant - No external dependencies added
- HOME sandboxing in all affected test files prevents leakage from developer environments
- The
if (key in flat)guard prevents arbitrary key injection fromdefaults.json - Nested key flattening correctly mirrors the try-block's
get()helper pattern Closes #1683present, issue hasapproved-enhancementlabel- Documentation update in CONFIGURATION.md is clear and accurate
Minor
- The
workflow.plan_check -> plan_checkermapping is correct but only covers one alias. If more aliases are added to the try-block in the future, this mapping will silently drift. Consider extracting the alias map to a shared constant. (Non-blocking, just a note.)
trek-e
added
the
review: changes requested
|
Closing — this was implemented by the maintainer in #1738. Thanks for the review feedback, it was incorporated into the final version. |
2 participants
Enhancement PR
Linked Issue
Closes #1683
What this enhancement improves
loadConfig()incore.cjs— the central config resolution used by all GSD commands.Before / After
Before:
When
.planning/config.jsonis missing,loadConfig()returns hardcoded defaults.~/.gsd/defaults.jsonis never consulted. Pre-project commands likemap-codebaseignore user globals entirely.After:
When
.planning/config.jsonis missing,loadConfig()reads~/.gsd/defaults.jsonas an intermediate fallback before falling back to hardcoded defaults.How it was implemented
get-shit-done/bin/lib/core.cjs— In thecatchblock ofloadConfig(), added~/.gsd/defaults.jsonreading with the same merge pattern used bybuildNewProjectConfig()inconfig.cjs. Supports both flat keys (model_profile) and nested keys (git.branching_strategy,workflow.research). Includesdepth→granularitymigration.docs/CONFIGURATION.md— Updated Global Defaults section to clarifydefaults.jsonapplies to all commands, not just/gsd-new-project. Added merge order documentation.tests/core.test.cjs— Added 3 new tests: defaults.json fallback, no-defaults.json fallback, nested key support. Sandboxed HOME inloadConfigdescribe block to isolate from developer's real~/.gsd/defaults.json.tests/commands.test.cjs— Sandboxed HOME inresolve-modeldescribe block for test isolation.tests/config.test.cjs��� Sandboxed HOME forconfig-ensure-sectiontest.Testing
How I verified the enhancement works
defaults.jsonvalues override hardcoded defaults when noconfig.jsonexistsconfig.jsonnordefaults.jsonexistsgit.*,workflow.*) are correctly resolved fromdefaults.jsonPlatforms tested
Runtimes tested
Scope confirmation
Checklist
Closes #NNN— PR will be auto-closed if missingapproved-enhancementlabel — PR will be closed if missingnpm test)Breaking changes
None. The merge order is additive — existing
.planning/config.jsonfiles take full precedence as before. Only the "no config.json exists" path changes, and that path currently returns hardcoded defaults which have no user-facing contract.